Add ability to pause/resume websocket without dumping subscriptions#1335
Add ability to pause/resume websocket without dumping subscriptions#1335designatednerd merged 7 commits intomainfrom
Conversation
designatednerd
force-pushed
the
add/websocket-pause
branch
from
bb51a2b to
2465544
Compare
designatednerd
force-pushed
the
add/websocket-pause
branch
from
2465544 to
10c3524
Compare
fassko
left a comment
fassko
left a comment
There was a problem hiding this comment.
Overall looks légit
About omitting self, I just like less code and use self` only where it is used.
| /// Reconnects a paused web socket. | ||
| /// | ||
| /// - Parameter autoReconnect: `true` if you want the websocket to automatically reconnect if the connection drops. Defaults to true. | ||
| public func resumeWebSocketConnection(autoReconnect: Bool = true) { |
There was a problem hiding this comment.
I think just reconnect is fine? Aligning it with Starscream. But on other hand adding auto can be more explicit. :P
| } | ||
|
|
||
| func webSocketTransportDidReconnect(_ webSocketTransport: WebSocketTransport) { | ||
| self.reconnectedExpectation?.fulfill() |
There was a problem hiding this comment.
| self.reconnectedExpectation?.fulfill() | |
| reconnectedExpectation?.fulfill() |
I don't know if there is a style guide, but I usually omit self when it's not needed.
There was a problem hiding this comment.
There is not currently.
Personally, I find self makes it much easier to disambiguate local vars from class vars when you're looking at things without syntax highlighting (ex, in Github). At some point I do want to make this more consistent throughout the codebase, but I'm gonna leave these for now.
There was a problem hiding this comment.
OK yep, being consistent is the key here, I agree!
| } | ||
|
|
||
| func webSocketTransport(_ webSocketTransport: WebSocketTransport, didDisconnectWithError error: Error?) { | ||
| self.disconnectedExpectation?.fulfill() |
There was a problem hiding this comment.
| self.disconnectedExpectation?.fulfill() | |
| disconnectedExpectation?.fulfill() |
| httpClient.perform(mutation: reviewMutation) { mutationResult in | ||
| switch mutationResult { | ||
| case .success: | ||
| break |
There was a problem hiding this comment.
Hmm how about already here it can fulfill the expectation? Or I'm missing something.
There was a problem hiding this comment.
The expectation should be fulfilled and the wait should be ended no matter which case comes in, so that's why it's below the switch statement
There was a problem hiding this comment.
I think XCTFail runs before I guess. Hmm ...
There was a problem hiding this comment.
Yep that's by design - we want that failure to occur before the wait ends.
| self.waitForSubscriptionsToStart() | ||
| sendReview() | ||
|
|
||
| self.disconnectedExpectation = self.expectation(description: "Web socket disconnected") |
There was a problem hiding this comment.
| self.disconnectedExpectation = self.expectation(description: "Web socket disconnected") | |
| disconnectedExpectation = expectation(description: "Web socket disconnected") |
|
|
||
| self.disconnectedExpectation = self.expectation(description: "Web socket disconnected") | ||
| webSocketTransport.pauseWebSocketConnection() | ||
| self.wait(for: [self.disconnectedExpectation!], timeout: 10) |
There was a problem hiding this comment.
| self.wait(for: [self.disconnectedExpectation!], timeout: 10) | |
| wait(for: [disconnectedExpectation!], timeout: 10) |
Do we need to specify exact expectation here?
There was a problem hiding this comment.
Yes since that's the only one we want to wait for
| // This should not go through since the socket is paused | ||
| sendReview() | ||
|
|
||
| self.reconnectedExpectation = self.expectation(description: "Web socket reconnected") |
There was a problem hiding this comment.
| self.reconnectedExpectation = self.expectation(description: "Web socket reconnected") | |
| reconnectedExpectation = expectation(description: "Web socket reconnected") |
| self.wait(for: [self.reconnectedExpectation!], timeout: 10) | ||
| self.waitForSubscriptionsToStart() |
There was a problem hiding this comment.
| self.wait(for: [self.reconnectedExpectation!], timeout: 10) | |
| self.waitForSubscriptionsToStart() | |
| wait(for: [reconnectedExpectation!], timeout: 10) | |
| waitForSubscriptionsToStart() |
| // Now that we've reconnected, this should go through to the same subscription. | ||
| sendReview() | ||
|
|
||
| self.wait(for: [subscriptionExpectation], timeout: 10) |
There was a problem hiding this comment.
| self.wait(for: [subscriptionExpectation], timeout: 10) | |
| wait(for: [subscriptionExpectation], timeout: 10) |
fassko
left a comment
fassko
left a comment
There was a problem hiding this comment.
Overall looks légit
About omitting self, I just like less code and use self` only where it is used.
Co-authored-by: Kristaps Grinbergs <fassko@gmail.com>
|
At some point I'll have to make a real decision about |
2 participants
Addresses #1333.
@fassko If you're around, I'd love your advice on whether I'm doing something terribly dumb with Starscream here 😇